-
Notifications
You must be signed in to change notification settings - Fork 587
newFOROP: fix crash when optimizing 2-var for over builtin::indexed #23429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: blead
Are you sure you want to change the base?
Conversation
f5e4805
to
1731242
Compare
When I apply the changes in the two test files to blead, configure and build, then run the two test files, they FAIL with segfaults. When I then apply the code changes, the test files PASS. So from a TDD perspective, this p.r. is good. Someone else will have to look at the concept and the C-code. |
use builtin qw( | ||
blessed | ||
ceil | ||
false | ||
floor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes in this file seem like they belong in a separate commit - they have nothing to do with fixing the builtin::indexed optimization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The builtin::indexed optimization works fine on builtin::indexed(...)
, but may crash on use builtin qw(indexed); ... indexed(...)
because the call optrees look different. I wanted to make sure both cases are optimized correctly (for all builtins).
This change is now a separate commit.
OP_ENTERSUB isn't necessarily a LISTOP, apparently, so we can't just grab its op_last. Instead, copy/paste logic from elsewhere in op.c to find the cvop. Also, avoid crashing on "fake" pad entries that represent lexical subs from outer scopes by climbing up the scope chain until we reach a real pad entry. Fixes Perl#23405.
When I saw (in Perl#23429) that use builtin qw(indexed); sub { for my ($x, $y) (indexed) {} } crashes, but sub { for my ($x, $y) (builtin::indexed) {} } works fine, I realized that optrees for calls to lexically imported subs look different from calls to package subs. This commit make sure both call variants are optimized to direct ops.
1731242
to
8eda65e
Compare
OP_ENTERSUB isn't necessarily a LISTOP, apparently, so we can't just grab its op_last. Instead, copy/paste logic from elsewhere in op.c to find the cvop.
Also, avoid crashing on "fake" pad entries that represent lexical subs from outer scopes by climbing up the scope chain until we reach a real pad entry.
Fixes #23405.